Skip to content

fix: ensure idempotent project-relative path rewriting in CommandRegistrar - #4553

Open
darion-yaphet wants to merge 1 commit into
github:mainfrom
darion-yaphet:fix/idempotent-path-rewriting
Open

fix: ensure idempotent project-relative path rewriting in CommandRegistrar#4553
darion-yaphet wants to merge 1 commit into
github:mainfrom
darion-yaphet:fix/idempotent-path-rewriting

Conversation

@darion-yaphet

@darion-yaphet darion-yaphet commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Description

`CommandRegistrar.rewrite_project_relative_paths()` previously relied on three consecutive `str.replace` calls followed by three separate `re.sub` passes, and

used .replace(".specify/.specify/", ".specify/").replace(".specify.specify/", ".specify/") as an ad-hoc post-processing fix. This "patch-on-patch" design was
fragile, caused redundant string scanning, and risked regressions when paths were processed multiple times.

This PR refactors the path normalization into a single unified regex match callback:
1. **Single-pass regex callback**: Consolidates multiple string and regex passes into one regex scan with a match handler.
2. **Native idempotency guard**: If a path prefix is already `.specify/`, it is returned unchanged without modification, preventing duplicate prefixing from the

source and eliminating the need for subsequent .replace() corrections.
3. Deterministic routing: Parent-relative paths (../) consistently route to root .specify/<target>/, while top-level / relative scripts/ preserve
extension-local scoping when extension_id is supplied.
4. Expanded delimiters: Recognizes common Markdown punctuation enclosures such as brackets [], parentheses (), braces {}, angle brackets <>, quotes,
and backticks.

Testing

<!-- How did you test your changes? -->

- [x] Ran existing tests with `pytest tests/test_extensions.py` (538 passed)
- [x] Ran new idempotency and delimiter test suite with `pytest tests/test_extensions.py -k "test_rewrite_project_relative_paths"` (5 passed)
- [x] Ran linter check with `ruff check src/specify_cli/agents.py tests/test_extensions.py` (0 errors)

## AI Disclosure

<!-- Per our Contributing guidelines, AI assistance must be disclosed. -->
<!-- See: https://github.com/github/spec-kit/blob/main/CONTRIBUTING.md#ai-contributions-in-spec-kit -->

- [ ] I **did not** use AI assistance for this contribution
- [x] I **did** use AI assistance (describe below)

@darion-yaphet
fix: ensure idempotent project-relative path rewriting in CommandRegi…
661957c 
…strar

Replace the fragile pattern of three sequential string replacements followed by three re.sub calls and trailing `.replace(".specify/.specify/", ".specify/")` / `.replace(".specify.specify/", ".specify/")` patches in CommandRegistrar.rewrite_project_relative_paths.

Consolidate the transformation into a unified regex match callback that:
- Inspects matched path prefixes (`.specify/`, `../`, `./`, `/`, or bare)
- Naturally guards already-normalized `.specify/` paths from double-prefixing
- Directs parent relative references (`../`) to root `.specify/<target>/`
- Preserves extension-local script scoping when extension_id is provided
- Expands boundary delimiters to include Markdown brackets, parentheses, braces, angle brackets, and backticks

Add unit tests in tests/test_extensions.py covering repeated passes for idempotency, markdown enclosure delimiters, and edge-case inputs.
@mnriem mnriem added triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-awaiting Waiting on author response labels Sep 12, 2026
@mnriem

mnriem commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

The delimiter and deeper-parent-path improvements have useful regression coverage. Before merge, please preserve parent-relative paths following =: --template=../../templates/spec.md and SCRIPT=../../scripts/bash/run.sh were rewritten previously but are now left unchanged. Add regression cases for these alongside the existing idempotency checks.

Please also clarify the description: the supplied samples already remain stable under repeated rewriting on the old code; the demonstrated improvements are routing and delimiter handling. Finally, complete this PR’s AI disclosure with the tool, model, mode/settings, and extent of assistance—the checkbox alone does not describe the contribution.

Drafted for @mnriem by GitHub Copilot (model: GPT-6 Astra; comment drafting).

@mnriem
mnriem requested a balanced review from Copilot September 15, 2026 15:46
Copilot AI reviewed Sep 15, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The refactor preserves routing behavior and includes focused regression coverage.

Pull request overview

Refactors project-relative path rewriting into an idempotent, single-pass implementation.

Changes:

  • Consolidates path normalization into one regex callback.
  • Adds regression coverage for idempotency, delimiters, and invalid inputs.
File summaries
File Description
src/specify_cli/agents.py Implements unified path rewriting.
tests/test_extensions.py Adds comprehensive regression tests.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@mnriem mnriem Awaiting requested review from mnriem mnriem is a code owner

Assignees

No one assigned

Labels

author-awaiting Waiting on author response author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@darion-yaphet @mnriem